Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing IsElem instance for NamedRoutes #1699

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

RaoulHC
Copy link
Contributor

@RaoulHC RaoulHC commented Aug 2, 2023

Added various tests for NamedRoutes too.

In writing the tests I realised that the client combinators // and /: can be used to create links in the same natural way.
I've added the combinators to the test suite for now, but it probably makes sense to put them somewhere in the servant package, though I wasn't sure where. Maybe just in the root API module, and then ensure that HasClient and Reexport in servant-client-core reexport them for any backwards compatibility?

Fixes #1674.

@RaoulHC
Copy link
Contributor Author

RaoulHC commented Aug 3, 2023

The CI build failure seems unrelated to my change and exactly the same as the one in #1698.
Couldn't reproduce it locally, though I wasn't able to get ghc-8.6.5 working on my system, so I've added a constraint to use the previous version for 8.6.5, though I'm not sure this will fix it. Running the same version of cabal locally (3.10.1.0), so it seems odd that it's failing to parse wreq.cabal.

cabal.project Outdated Show resolved Hide resolved
@ysangkok
Copy link
Contributor

This looks good but it is missing a changelog entry. See the other PRs for how to make one.

@RaoulHC
Copy link
Contributor Author

RaoulHC commented Aug 11, 2023

Added a changelog entry, any thoughts on where the // and /: might belong in servant?

@ysangkok
Copy link
Contributor

I think that people will only appreciate them if they see examples. And if they see examples, it almost doesn't matter what the name of the combinator is. Personally, I think it is unnecessary since their definitions are trivial. There is no reason to abstract away function application.

changelog.d/1699 Outdated Show resolved Hide resolved
@RaoulHC
Copy link
Contributor Author

RaoulHC commented Aug 11, 2023

I think that people will only appreciate them if they see examples. And if they see examples, it almost doesn't matter what the name of the combinator is. Personally, I think it is unnecessary since their definitions are trivial. There is no reason to abstract away function application.

While // is just a redefinition of &, /: does make it more ergonomic imo. Either way, these helpers are already defined in Servant.Client.Core.HasClient, I just think it might sense to move them to the servant package. If we do that then the documentation should be updated to have examples for both links and clients.

@ysangkok ysangkok merged commit 809ca37 into haskell-servant:master Aug 11, 2023
@ysangkok
Copy link
Contributor

You could submit a separate pull request for that, for now I will merge this since it fixes CI and the instance is useful even without the combinators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making query parameters optionals in allLinks/safeLink with NamedRoutes
2 participants